Add multipart request support #65

pucinsk · 2019-10-31T11:14:10Z

This PR allows send files via graphql. It adds FaradayMultipartAdapter to allow send multipart requests via graphlient gem.
It accepts File instances and convert its to Faraday::UploadIO before sending.

dangerpr-bot · 2019-10-31T11:16:29Z

	1 Warning
⚠️	Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#65](https://github.com/ashkan18/graphlient/pull/65): Add multipart request support - [@pucinsk](https://github.com/pucinsk).

Generated by 🚫 Danger

dblock

This is pretty cool, but it seems strange to me that you would have to use a different adapter to send a file. Shouldn't I be able to do that by default?

README.md

spec/graphlient/adapters/http/faraday_multipart_adapter/format_multipart_variables_spec.rb

yuki24 · 2019-10-31T14:33:29Z

graphlient.gemspec

@@ -17,4 +17,5 @@ Gem::Specification.new do |s|
  s.add_dependency 'faraday'
  s.add_dependency 'faraday_middleware'
  s.add_dependency 'graphql-client'
+  s.add_dependency 'mime-types'


Gems related to mime types are known to allocate excessive memory. Have you looked into the mini_mime gem?

Oh, I haven't thought about it.
Yup, mini_mime should work as expected too.

And what do you think if we try to load mime types lib from users environment and if he does not have any mime types library then we throw an expection and enforce user to install mime types lib with a message_No mime types library detected you must add to your Gemfile mime-types or mini_mime gem_?
Code could look something like that:

def mime_type(file) if defined?(MIME::TYPES) MIME::TYPES.type_for(file.path) if defined?(MiniMyme) MiniMyme.lookup_by_filename(file.path) else raise Graphlient::Errors::NoMimeTypesLibError, 'Please add `mime-types` or `mini_mime` gem to your Gemfile' end end

It would nice for Rails users because Rails already includes mime-types gem.

That sounds like a good idea 👍

yuki24 · 2019-10-31T14:34:14Z

lib/graphlient/adapters/http/faraday_multipart_adapter.rb

+
+          response.body
+        rescue Faraday::ClientError => e
+          raise Graphlient::Errors::FaradayServerError, e


It doesn't seem right to translate client errors to server errors. Also, you don't have to pass in the e to the second argument as Ruby captures the cause and users have access to it with exception#cause.

oh, it was shamefully copy-pasted from FaradayAdapter
So I cannot tell why it does looks so.

yuki24 · 2019-10-31T14:47:18Z

spec/graphlient/adapters/http/faraday_multipart_adapter/format_multipart_variables_spec.rb

+      let(:variables) { { val: { file: File.new('spec/support/fixtures/empty.txt') } } }
+
+      it 'contverts file to Faraday::UploadIO' do
+        expect(call[:val][:file]).to be_a(Faraday::UploadIO)


In Ruby, duck typing is more preferable rather than checking classes.

ashkan18

Thanks for adding this and also adding specs! Looks awesome! 😍

Just have the same question as @dblock about possibly not needing a separate adapter for this and have Faraday middleware handling this. I wonder if its sufficient to just set proper header for multipart requests and have Faraday handle it automatically.

ashkan18 · 2019-10-31T19:34:44Z

lib/graphlient/adapters/http/faraday_multipart_adapter/format_multipart_variables.rb

+
+          def file_variable_value(file)
+            content_type = MIME::Types.type_for(file.path).first
+            return Faraday::UploadIO.new(file.path, content_type) if content_type


should we use Faraday::FilePart or Faraday::ParamPart based on this doc?

Mhm. Good point. Faraday::UploadIO will be deprecated. 😞 But in Faraday v0.17.0 UploadIO is only valid way to add files to request.
I think the next Faraday version will v1.0 and it will have Faraday::Parts module where all parts of request will be described.
Maybe we should specify faraday gem version in Gemfile just to make sure that for now Faraday::UploadIO could work?

dblock · 2019-11-06T13:07:07Z

@pucinsk My biggest reservation about this PR is the creation of another adapter. Can we build multipart support into the existing one? What's the downside?

pucinsk · 2019-11-07T14:27:04Z

There are no downsides. I thought that it could be optional adapter.
But if it would be better that files supporting would be enabled by default than I am more than willing to add this to main FaradayAdapter

ashkan18 · 2019-11-07T14:45:47Z

Thanks @pucinsk, yes that would be awesome to add support to existing adapter. Thanks so much!

dblock · 2019-11-07T20:26:59Z

Yes, 1 adapter > 2 adapters.

uiltonlopes · 2022-07-22T23:08:09Z

Hi can you merge the branch? I would like to use this functionality

dblock

Please see above. From the user POV a separate adapter is too difficult to use, as it requires creating a separate client to upload files from other requests. The ask is to integrate support for multipart requests into the existing one. Happy to merge something along those lines.

uiltonlopes · 2022-07-23T14:15:12Z

This is pretty cool, but it seems strange to me that you would have to use a different adapter to send a file. Shouldn't I be able to do that by default?

Alright master. Can you help me with some file upload example that still manages to leverage the gem

dblock · 2022-07-23T14:26:46Z

This is pretty cool, but it seems strange to me that you would have to use a different adapter to send a file. Shouldn't I be able to do that by default?

Alright master. Can you help me with some file upload example that still manages to leverage the gem

I've never done that myself, so don't know how to do it with the current implementation. If you want to try and refactor this PR to support multipart I can try to help out fix any remaining issues on something that (almost) works.

PedroAugustoRamalhoDuarte · 2023-12-22T17:25:44Z

Hello guys, is there any way to send files with Graphlient, or only with this PR?

dblock · 2023-12-22T19:34:55Z

I don't know of one.

Jokūbas Pučinskas added 3 commits October 31, 2019 11:24

Add multipart requests support

32dcce0

Add documentation for multipart request support

cc7096c

Add rubocop_todo

9783a3d

dblock reviewed Oct 31, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

spec/graphlient/adapters/http/faraday_multipart_adapter/format_multipart_variables_spec.rb Outdated Show resolved Hide resolved

yuki24 reviewed Oct 31, 2019

View reviewed changes

ashkan18 reviewed Oct 31, 2019

View reviewed changes

Jokūbas Pučinskas added 2 commits November 6, 2019 13:57

Fix typo

6ea240c

Do not use ruby2.4 "Hash#transform_values" method

22f0492

pucinsk requested review from yuki24, ashkan18 and dblock November 6, 2019 11:59

dblock mentioned this pull request Jun 11, 2022

fix: raise ExecutionError with partial error response #87

Merged

dblock requested changes Jul 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multipart request support #65

Add multipart request support #65

pucinsk commented Oct 31, 2019 •

edited

Loading

dangerpr-bot commented Oct 31, 2019 •

edited

Loading

dblock left a comment

yuki24 Oct 31, 2019

pucinsk Nov 6, 2019

yuki24 Nov 6, 2019

yuki24 Oct 31, 2019 •

edited

Loading

pucinsk Nov 6, 2019

yuki24 Oct 31, 2019 •

edited

Loading

ashkan18 left a comment

ashkan18 Oct 31, 2019

pucinsk Nov 6, 2019 •

edited

Loading

dblock commented Nov 6, 2019

pucinsk commented Nov 7, 2019

ashkan18 commented Nov 7, 2019

dblock commented Nov 7, 2019

uiltonlopes commented Jul 22, 2022

dblock left a comment •

edited

Loading

uiltonlopes commented Jul 23, 2022

dblock commented Jul 23, 2022

PedroAugustoRamalhoDuarte commented Dec 22, 2023

dblock commented Dec 22, 2023

Add multipart request support #65

Are you sure you want to change the base?

Add multipart request support #65

Conversation

pucinsk commented Oct 31, 2019 • edited Loading

dangerpr-bot commented Oct 31, 2019 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

yuki24 Oct 31, 2019

Choose a reason for hiding this comment

pucinsk Nov 6, 2019

Choose a reason for hiding this comment

yuki24 Nov 6, 2019

Choose a reason for hiding this comment

yuki24 Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

pucinsk Nov 6, 2019

Choose a reason for hiding this comment

yuki24 Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

ashkan18 left a comment

Choose a reason for hiding this comment

ashkan18 Oct 31, 2019

Choose a reason for hiding this comment

pucinsk Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

dblock commented Nov 6, 2019

pucinsk commented Nov 7, 2019

ashkan18 commented Nov 7, 2019

dblock commented Nov 7, 2019

uiltonlopes commented Jul 22, 2022

dblock left a comment • edited Loading

Choose a reason for hiding this comment

uiltonlopes commented Jul 23, 2022

dblock commented Jul 23, 2022

PedroAugustoRamalhoDuarte commented Dec 22, 2023

dblock commented Dec 22, 2023

pucinsk commented Oct 31, 2019 •

edited

Loading

dangerpr-bot commented Oct 31, 2019 •

edited

Loading

yuki24 Oct 31, 2019 •

edited

Loading

yuki24 Oct 31, 2019 •

edited

Loading

pucinsk Nov 6, 2019 •

edited

Loading

dblock left a comment •

edited

Loading